Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: make it easier to configure environment runner #18273

Merged
merged 2 commits into from
Oct 7, 2024
Merged

fix: make it easier to configure environment runner #18273

merged 2 commits into from
Oct 7, 2024
+15 −10

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Oct 3, 2024 •
edited
Loading

Description

Hide the complexity of the custom runner configuration. If you need to configure the runner, you will most likely use the createServerModuleRunner. It just has a single option argument, let's pass it down directly instead of requiring the user to import the function - they shouldn't even need to know it exists.

@stackblitz StackBlitz
Copy link

stackblitz bot commented Oct 3, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red sapphi-red added the feat: environment API Vite Environment API label Oct 4, 2024
@sapphi-red
Copy link
Member

Won't this make it impossible to swap the ModuleRunner while keeping isRunnableDevEnvironment(devEnv) to be true?

@sheremet-va
Copy link
Member Author

sheremet-va commented Oct 4, 2024 •
edited
Loading

Won't this make it impossible to swap the ModuleRunner while keeping isRunnableDevEnvironment(devEnv) to be true?

What do you mean by "swap"? It always requires the ModuleRunner instance. The createServerModuleRunner/new ModuleRunner creates that runner. The runnerOptions can configure any options that you can already configure by calling new ModuleRunner:

vite/packages/vite/src/node/ssr/runtime/serverModuleRunner.ts

Line 16 in 51bb168

export interface ServerModuleRunnerOptions

This PR just hides the complexity of calling the createServerModuleRunner/new ModuleRunner directly. I've noticed in a lot of feedback that the new APIs feel too "boilerplate"-y, this PR tries to remove that friction and make it easier to work with

Basically, what I am saying is that runner: () => ModuleRunner and runnerOptions are equivalent in power, but one is easier to use than the other.

@sapphi-red
Copy link
Member

What do you mean by "swap"?

I mean using a custom ModuleRunner that extends the ModuleRunner class. I guess it's allowed to be used like that, reading this comment.

vite/packages/vite/src/module-runner/runner.ts

Line 291 in a99a0aa

// override is allowed, consider this a public API

I think this PR makes it more easier and that is good, but I guess we need to give a way to declare a RunnableDevEnvironment with a custom ModuleRunner.

@sheremet-va
Copy link
Member Author

I see what you mean now. I will revert the runner change and also add the options as the argument in the factory so they are not lost.

@patak-dev patak-dev merged commit fb35a78 into vitejs:main Oct 7, 2024
11 of 12 checks passed
@sheremet-va sheremet-va deleted the fix/make-easier-to-configure-runnable-env branch October 7, 2024 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patak-dev patak-dev patak-dev approved these changes

@hi-ogawa hi-ogawa hi-ogawa approved these changes

@sapphi-red sapphi-red sapphi-red approved these changes

Assignees
No one assigned
Labels
feat: environment API Vite Environment API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sheremet-va @sapphi-red @patak-dev @hi-ogawa